Coverage increased (+0.04%) to 85.739% when pulling fafd74c on SeesePlusPlus:emit-new-contract-event into 1394a2e on ethereumjs:master.

Hi @seesemichaelj, thanks for the PR, looks really exciting what you are working on! I am generally positive towards this PR and my first impression is that both naming and implementation makes sense.

Want to dig a bit deeper though and also let this sink in for a few days. If someone else from the team (or actually anyone who has a look into this) have some comments, this would be also helpful!

Happy real-time debugging!
Holger

Sounds great thanks!

Hi @seesemichaelj, I now tested this locally within the run-transactions-complete example by adding an event listener to the index.js file like:

var vm = new VM({state: stateTrie, emitNewContractEvents: true })

//...

vm.on('newContract', function(object, callback) {
  console.log(object)
  callback()
})

This works and blocks when I remove the callback() execution. It doesn't block though when I remove the whole listener. I am not completely understanding why, is this intended behavior respectively can you explain that (so if I completely remove the code from above, the example script runs to the end again)?

Two thoughs on this, still thinking about the API:
a) if it doesn't block when not used and if this is intended: is it really necessary to make this event an option in the VM initialization or couldn't this just be emitted by default?
b) if we keep the option: would it make sense, to make the API on this more general and from the start create a structure where further events can easily be added, e.g. by doing a more generic API option opts.activatedEventList (or similar) and instantiate with e.g. { activatedEventList: ['newContract'] }?

Thanks @holgerd77 for looking into this!

Before responding, I'm going to look into a couple of things. I think I may have misunderstood some things during implementation.

It definitely was intended to block. However a cleaner solution would be "only block if someone is listening on that topic."

I'll get back to you once I gather some more information on this.

This is actually really strange: if I comment out callback() in the event listener it stops. If I am removing the complete event listener, it goes on again. Would a bit assume that there is some Node-internal stuff going on here, some kind of implicit/hidden (and maybe? accidental) callback execution?

Anyway: I think we can't (can we?) really trust this stuff. This actually would be somewhat our desired behavior, but I am not feeling really save on this.

Here is the Node source code:
https://github.com/nodejs/node-v8/blob/canary/lib/events.js#L140

Not sure how deep one want to dig into that.

@holgerd77 Eek! My code is written for normal EventEmitter's, but apparently ethereumjs-vm uses https://github.com/ahultgren/async-eventemitter. And the first bullet on the readme:

Data sent to event listeners (eg emit(data)) must always be zero or one argument, and can not be a function.

and the second bullet:

The second argument can only be a callback, and will only be supplied if the event listener has an arity of two or more (eg function(e, next){}).

and my code:

self.emit('newContract', {
  address: createdAddress,
  code: code
}, callback)

😁

I'm going to fix this and get back to you when it's ready for review

Ok @holgerd77 I have more information now!

• Turns out the emitNewContractEvents option is not necessary. The AsyncEventEmitter library (https://github.com/ahultgren/async-eventemitter/blob/master/lib/AsyncEventEmitter.js#L19 is the emit function) will:
  • Call the callback if there are no listeners
  • For every listener, it will check the arity; if it is less than 2 (aka it doesn't have the callback as an argument to it's listener function), it just moves along as if it was a synchronous function
  • After all the listeners call the callback or, if an arity of <2, finish synchronous execution, the emit's callback will be called

As far as your comment: if I comment out callback() in the event listener it stops, I'm really confused what's going on with this. I thought I had a handle on it, but after more thinking, I'm more confused haha.

If I add a setInterval(() => {}, 500) to the bottom of the file, but have the callback() commented out (but still specified in the callback function args), then the program doesn't exit. This makes me think the AsyncEventEmitter isn't causing it to exit prematurely.

Without the setInterval I checked that the exit code is 0 so it's not error'ing out/crashing.

If I use a setTimeout(callback, 5000) instead of callback(), the program does wait and then carries on as expected. This makes me think that it's not the fact the function is stopping the program when it finishes it's synchronous execution.

I'm not really sure how to proceed on that front; changing it to the below still has the issue, so I don't believe this issue is related to this PR.

vm.on('step', function(object, callback) {
  console.log(object)
  //setTimeout(callback, 5000)
})
``

I went ahead and removed the flag as it's not necessary per my prior comment. Let me know what you think about the other issue (that I think is unrelated), but as far as I'm concerned, this is now ready for re-review. Thanks!

@holgerd77 I went ahead and updated the section to be consistent and accurate. Looks like it was missing some extra events in the table of contents part, so I added those as well. Let me know if there are any other requested changes. Thanks!

@seesemichaelj Thanks, cool, didn't want to be to picky on this - think our README also has other constistency lacks - but this just caught the eye so directly. 😄

Not urgent. I have one more PR coming to support Velma completely, so I won't be using the official ethereumjs-vm module until that's incorporated as well. From my perspective, you could wait until after that!

I should have that PR posted in the next day or two.

Ok.